Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ENH: ImageRegion support C++17 structured binding #4367

Merged
merged 1 commit into from
Dec 26, 2023
Merged

ENH: ImageRegion support C++17 structured binding #4367

merged 1 commit into from
Dec 26, 2023

Conversation

N-Dekker
Copy link
Contributor

Added get() member functions and specializations of std::tuple_size and std::tuple_element for ImageRegion, in order to support structured binding, for example:

auto [index, size] = image.GetRequestedRegion();

@github-actions github-actions bot added type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Dec 12, 2023
@N-Dekker N-Dekker force-pushed the ImageRegion-structured-binding branch from 0a73924 to b2f8338 Compare December 12, 2023 14:36
@N-Dekker N-Dekker mentioned this pull request Dec 12, 2023
Open
@N-Dekker
Copy link
Contributor Author

@dzenanz Thanks for you approval! I think it would be pretty cool if ImageRegion would support structured binding. It feels a bit Pythonic 😃

Unfortunately the code currently appears to trigger some undeserved clang-tidy warnings, saying modification of 'std' namespace can result in undefined behavior [cert-dcl58-cpp], as I mentioned at llvm/llvm-project#45454 (comment) Even though the code appears correct, as confirmed by @Febbe at llvm/llvm-project#45454 (comment)

Shall I locally disable this specific Clang-Tidy warning, by adding something like //NOLINTNEXTLINE(cert-dcl58-cpp) to those specific lines of code? Or shall we just leave it this way for now, as long as our CI is happy?

@dzenanz
Copy link
Member

dzenanz commented Dec 12, 2023

Suppressing the warning sounds better to me.

@N-Dekker N-Dekker force-pushed the ImageRegion-structured-binding branch 2 times, most recently from e464883 to bf37e85 Compare December 12, 2023 18:42
@github-actions github-actions bot added the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label Dec 12, 2023
@N-Dekker N-Dekker force-pushed the ImageRegion-structured-binding branch from bf37e85 to 2e98742 Compare December 12, 2023 18:56
@N-Dekker N-Dekker changed the title ENH: ImageRegion support C++17 structured binding ENH: ImageRegion support C++17 structured binding Dec 13, 2023
@N-Dekker N-Dekker force-pushed the ImageRegion-structured-binding branch from 2e98742 to 5cf540e Compare December 13, 2023 19:29
@N-Dekker
ENH: ImageRegion support C++17 structured binding
c8c9983 
Added `get()` member functions and specializations of `std::tuple_size` and
`std::tuple_element` for ImageRegion, in order to support structured binding,
for example:

    auto [index, size] = image.GetRequestedRegion();

Suppressed the undeserved Clang-Tidy "cert-dcl58-cpp" warnings that appeared
when specializing those `std` templates.
@N-Dekker N-Dekker force-pushed the ImageRegion-structured-binding branch from 5cf540e to c8c9983 Compare December 13, 2023 19:48
@N-Dekker N-Dekker marked this pull request as ready for review December 13, 2023 20:00
@N-Dekker
Copy link
Contributor Author

@thewtex Although this is a rather small PR, I think it has a certain "cool factor", allowing users to do auto [index, size] = region, so maybe you can consider mentioning the feature explicitly in the next release notes 😃 At least, if it is still in time for ITK 5.4, of course!

thewtex
thewtex approved these changes Dec 16, 2023
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@N-Dekker very, very cool! 😎

Yes, I am making a mental note for the 5.4rc3 release notes.

@hjmjohnson hjmjohnson merged commit 72aa9a6 into InsightSoftwareConsortium:master Dec 26, 2023
12 checks passed
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 27, 2023
@N-Dekker
STYLE: Use ImageRegion auto [index, size] = region structured bindings
9285c3e 
Replaced initializations of the form:

    SizeType size = region.GetSize();
    IndexType index = region.GetIndex();

By using structured binding of the form `auto [index, size] = region`, as was
introduced by pull request InsightSoftwareConsortium#4367
commit 72aa9a6 "ENH: `ImageRegion` support C++17
structured binding"

Did so by Notepad++ Replace in Files, using regular expressions:

    Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetSize\(\);$
    Replace with: $1const auto [$3, $5] = $4;

    Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetIndex\(\);$
    Replace with: $1const auto [$5, $3] = $4;

    Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetSize\(\);$
    Replace with: $1auto [$3, $5] = $4;

    Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetIndex\(\);$
    Replace with: $1auto [$5, $3] = $4;
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 27, 2023
@N-Dekker
STYLE: Use ImageRegion auto [index, size] = region structured bindings
8ed2f28 
Replaced initializations of the form:

    SizeType size = region.GetSize();
    IndexType index = region.GetIndex();

By using structured binding of the form `auto [index, size] = region`, as was
introduced by pull request InsightSoftwareConsortium#4367
commit 72aa9a6 "ENH: `ImageRegion` support C++17
structured binding"

Did so by Notepad++ Replace in Files, using regular expressions:

    Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetSize\(\);$
    Replace with: $1const auto [$3, $5] = $4;

    Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetIndex\(\);$
    Replace with: $1const auto [$5, $3] = $4;

    Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetSize\(\);$
    Replace with: $1auto [$3, $5] = $4;

    Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetIndex\(\);$
    Replace with: $1auto [$5, $3] = $4;

Manually added `const` to bindings in `SetBound(const SizeType &)` member
functions, and excluded `ImageIORegion`.
@N-Dekker N-Dekker mentioned this pull request Dec 27, 2023
Closed
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Jan 1, 2024
@N-Dekker
STYLE: Use ImageRegion auto [index, size] = region structured bindings
4598b02 
Replaced initializations of the form:

    SizeType size = region.GetSize();
    IndexType index = region.GetIndex();

By using structured binding of the form `auto [index, size] = region`, as was
introduced by pull request InsightSoftwareConsortium#4367
commit 72aa9a6 "ENH: `ImageRegion` support C++17
structured binding"

Did so by Notepad++ Replace in Files, using regular expressions:

    Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetSize\(\);$
    Replace with: $1const auto [$3, $5] = $4;

    Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetIndex\(\);$
    Replace with: $1const auto [$5, $3] = $4;

    Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetSize\(\);$
    Replace with: $1auto [$3, $5] = $4;

    Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetIndex\(\);$
    Replace with: $1auto [$5, $3] = $4;

Manually added `const` to bindings in `SetBound(const SizeType &)` member
functions, excluded `ImageIORegion`, and removed `Index` and `Size` type aliases
that are no longer used after this commit.
@dzenanz
Copy link
Member

dzenanz commented Jan 2, 2024

Did this PR cause these compile errors:
https://open.cdash.org/viewBuildError.php?buildid=9269183

@issakomi
Copy link
Member

issakomi commented Jan 2, 2024

Did this PR cause these compile errors: https://open.cdash.org/viewBuildError.php?buildid=9269183

static_asserts are from #4373. BTW users add rather often -DITK_FUTURE_LEGACY_REMOVE:BOOL=ON too, even if it is for developers only, e.g. here.

@N-Dekker N-Dekker mentioned this pull request Jan 2, 2024
Merged
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Jan 14, 2024
@N-Dekker
STYLE: Use ImageRegion auto [index, size] = region structured bindings
b96bd57 
Replaced initializations of the form:

    SizeType size = region.GetSize();
    IndexType index = region.GetIndex();

By using structured binding of the form `auto [index, size] = region`, as was
introduced by pull request InsightSoftwareConsortium#4367
commit 72aa9a6 "ENH: `ImageRegion` support C++17
structured binding"

Did so by Notepad++ Replace in Files, using regular expressions:

    Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetSize\(\);$
    Replace with: $1const auto [$3, $5] = $4;

    Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetIndex\(\);$
    Replace with: $1const auto [$5, $3] = $4;

    Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetSize\(\);$
    Replace with: $1auto [$3, $5] = $4;

    Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetIndex\(\);$
    Replace with: $1auto [$5, $3] = $4;

Manually added `const` to bindings in `SetBound(const SizeType &)` member
functions, excluded `ImageIORegion`, and removed `Index` and `Size` type aliases
that are no longer used after this commit.
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Feb 9, 2024
@N-Dekker
COMP: Locally ignore Clang -Wmismatched-tags warnings in ImageRegion
6e9abfa 
After pull request InsightSoftwareConsortium#4367
commit 72aa9a6 (ENH: `ImageRegion` support
C++17 structured binding), a few unimportant `-Wmismatched-tags` warnings
appeared from Mac10.13-AppleClang-rel-x86_64, at
https://open.cdash.org/build/9256915

This commit locally disables those warnings, followed:
https://github.com/Naios/continuable/blob/121265df7123cf672ea8db917eaca2d0fbd9aef5/include/continuable/continuable-result.hpp#L291-L308

Reported to me by mail, from Dženan Zukić and Sean McBride.
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Feb 9, 2024
@N-Dekker
COMP: Locally ignore Clang -Wmismatched-tags warnings in ImageRegion
117385b 
After pull request InsightSoftwareConsortium#4367
commit 72aa9a6 (ENH: `ImageRegion` support
C++17 structured binding), a few unimportant `-Wmismatched-tags` warnings
appeared from Mac10.13-AppleClang-rel-x86_64, at
https://open.cdash.org/build/9256915

This commit locally disables those warnings, followed:
https://github.com/Naios/continuable/blob/121265df7123cf672ea8db917eaca2d0fbd9aef5/include/continuable/continuable-result.hpp#L291-L308

Reported to me by mail, from Dženan Zukić and Sean McBride.
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Feb 9, 2024
@N-Dekker
COMP: Locally ignore Clang -Wmismatched-tags warnings in ImageRegion
b512639 
After pull request InsightSoftwareConsortium#4367
commit 72aa9a6 (ENH: `ImageRegion` support
C++17 structured binding), a few unimportant `-Wmismatched-tags` warnings
appeared from Mac10.13-AppleClang-rel-x86_64, at
https://open.cdash.org/build/9256915

This commit locally disables those warnings, followed:
https://github.com/Naios/continuable/blob/121265df7123cf672ea8db917eaca2d0fbd9aef5/include/continuable/continuable-result.hpp#L291-L308

Reported to me by mail, from Dženan Zukić and Sean McBride.
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Feb 9, 2024
@N-Dekker
COMP: Locally ignore Clang -Wmismatched-tags warnings in ImageRegion
bdf512b 
After pull request InsightSoftwareConsortium#4367
commit 72aa9a6 (ENH: `ImageRegion` support
C++17 structured binding), a few unimportant `-Wmismatched-tags` warnings
appeared from Mac10.13-AppleClang-rel-x86_64, at
https://open.cdash.org/build/9256915 as was reported to me by mail, from Dženan
Zukić and Sean McBride.

This commit locally disables those warnings, following
https://github.com/Naios/continuable/blob/121265df7123cf672ea8db917eaca2d0fbd9aef5/include/continuable/continuable-result.hpp#L291-L308
by Denis Blank.
@N-Dekker N-Dekker mentioned this pull request Feb 9, 2024
Merged
dzenanz pushed a commit that referenced this pull request Feb 12, 2024
@N-Dekker @dzenanz
COMP: Locally ignore Clang -Wmismatched-tags warnings in ImageRegion
9a35c2f 
After pull request #4367
commit 72aa9a6 (ENH: `ImageRegion` support
C++17 structured binding), a few unimportant `-Wmismatched-tags` warnings
appeared from Mac10.13-AppleClang-rel-x86_64, at
https://open.cdash.org/build/9256915 as was reported to me by mail, from Dženan
Zukić and Sean McBride.

This commit locally disables those warnings, following
https://github.com/Naios/continuable/blob/121265df7123cf672ea8db917eaca2d0fbd9aef5/include/continuable/continuable-result.hpp#L291-L308
by Denis Blank.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@issakomi issakomi issakomi left review comments

@dave3d dave3d dave3d left review comments

@dzenanz dzenanz dzenanz approved these changes

@hjmjohnson hjmjohnson hjmjohnson approved these changes

@thewtex thewtex thewtex approved these changes

Assignees
No one assigned
Labels
area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@N-Dekker @dzenanz @issakomi @thewtex @hjmjohnson @dave3d